Skip to content

1874 failed attch ios trail#415

Open
S-S-T wants to merge 46 commits intodevelopfrom
1874-failed-attch-ios-trail
Open

1874 failed attch ios trail#415
S-S-T wants to merge 46 commits intodevelopfrom
1874-failed-attch-ios-trail

Conversation

@S-S-T
Copy link
Copy Markdown
Contributor

@S-S-T S-S-T commented Apr 21, 2026

Mobile Client Compatibility — Failed Attachment Handling (iOS/Android) #1874

Summary

This branch implements the server-side API contract for failed attachment handling following ClamAV pre-disk scanning, establishing the foundation for mobile client compatibility.


Changes Made

  • Modified service/src/adapters/observations/adapters.observations.controllers.web.ts to handle ClamAV rejected attachments by storing a placeholder image (SecError.png) in place of the rejected file and returning a structured response including contentStored: false, error, and failures fields
  • Added service/src/assets/SecError.png — placeholder image displayed when an attachment is rejected by ClamAV
  • Merged ClamAV pre-disk scanning work from branch predisk-scanning-1756

Response Structure on Rejection

{
    "id": "",
    "fieldName": "",
    "observationFormId": "",
    "lastModified": "",
    "contentType": "",
    "name": "",
    "size": "",
    "oriented": "",
    "thumbnails": [],
    "contentStored": false,
    "url": "",
    "relativePath": "",
    "error": "File failed security scan",
    "failures": [
        {
            "file": "eicar.txt",
            "error": "File failed security scan"
        }
    ]
}

Testing

  • Tested locally with clean file upload — success response correct
  • Tested locally with eicar.txt — rejection response matches agreed structure
  • Verified on test environment with ClamAV running

Dependencies

  • Requires ClamAV running with CLAM_AV_URL env variable set
  • Branched from develop with predisk-scanning-1756 merged in

Open Items

  • iOS/Android client testing against new response structure pending
  • Granular failure messaging on mobile TBD pending iOS/Android findings

Sanford Schaffer and others added 30 commits February 19, 2026 06:47
…attachments, configure node debugger, etc..
…canned and cleanly running thru clamav with no errors..
…ully resolve failures in clamav instead of crashing..
…ded, need to keep app afloat, this finishes 1753 ticket..
Copy link
Copy Markdown
Contributor

@paulsolt-ofsw paulsolt-ofsw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I tried again from iOS the file is not removed on mobile.

<test.mage.geointapps.com>

2026-4-21.ClamAV.code.review.mp4

If we don't update the lastModified data on the server, than the iOS app never attempts to update things. Something feels off around this logic for swapping to a placeholder image.

I still have the "infected" file in my local storage on iOS, and I never see the "security image".

There are a lot of changes on this PR that don't appear to be related to the actual work.

I'm a little concerned to merge this as is, because of the scope of changes, and chance for regressions.

Comment thread .vscode/launch.json
Comment thread instance/package.json
Comment on lines -27 to +32
"@ngageoint/mage.arcgis.service": "../plugins/arcgis/service",
"@ngageoint/mage.arcgis.web-app": "../plugins/arcgis/web-app/dist/main",
"@ngageoint/mage.sftp.web": "../plugins/sftp/web/dist/main",
"@ngageoint/mage.sftp.service": "../plugins/sftp/service",
"@ngageoint/mage.nga-msi": "../plugins/nga-msi",
"@ngageoint/mage.arcgis.service": "file:../plugins/arcgis/service",
"@ngageoint/mage.arcgis.web-app": "file:../plugins/arcgis/web-app/dist/main",
"@ngageoint/mage.nga-msi": "file:../plugins/nga-msi",
"@ngageoint/mage.service": "../service",
"@ngageoint/mage.web-app": "../web-app/dist"
"@ngageoint/mage.sftp.service": "file:../plugins/sftp/service",
"@ngageoint/mage.sftp.web": "file:../plugins/sftp/web/dist/main",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change these, is it related to the actual work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were pulled in automatically during a rebuild on preceding branch 1756, not an intentional dependency addition. The key commit is b88a899f

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so npm install or something changed this to use file:?

Why doesn't this "../web-app/dist" get the same treatment? Does it break something in the build/install?

    "@ngageoint/mage.sftp.web": "file:../plugins/sftp/web/dist/main", 
    "@ngageoint/mage.web-app": "../web-app/dist",

Comment thread service/src/assets/SecError.png
Comment thread docker-compose.yml
Comment thread docker-compose.yml
@S-S-T
Copy link
Copy Markdown
Contributor Author

S-S-T commented Apr 22, 2026

Merging this PR is still a ways off. At this point, not comfortable--and won't be until there is requisite clarity and proof. Furthermore, the PR's scope is incomplete/incorrect. The modifiedDate issue must be addressed in order for iOS processes to flow as intended. The showing of the canned image is still unclear and may require further discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants